Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update sdk #201

Merged
merged 2 commits into from
Apr 1, 2022
Merged

Update sdk #201

merged 2 commits into from
Apr 1, 2022

Conversation

forsyth2
Copy link
Collaborator

Update globus_sdk. Resolves #196

@forsyth2 forsyth2 added DevOps CI/CD, configuration, etc. Globus Globus labels Mar 17, 2022
@forsyth2 forsyth2 self-assigned this Mar 17, 2022
@forsyth2
Copy link
Collaborator Author

Running python -m unittest tests/test_*.py in environment without these changes:

.
----------------------------------------------------------------------
Ran 62 tests in 585.516s

OK

And with these changes:

.
======================================================================
ERROR: testLs (tests.test_globus.TestGlobus)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/global/cscratch1/sd/forsyth/zstash/tests/test_globus.py", line 182, in testLs
    "testLsGlobus", f"globus://{hpss_globus_endpoint}/~/zstash_test/"
  File "/global/cscratch1/sd/forsyth/zstash/tests/test_globus.py", line 159, in helperLsGlobus
    self.preactivate_globus()
  File "/global/cscratch1/sd/forsyth/zstash/tests/test_globus.py", line 62, in preactivate_globus
    native_client.login(no_local_server=True, refresh_tokens=True)
  File "/global/homes/f/forsyth/miniconda3/envs/zstash_issue196/lib/python3.7/site-packages/fair_research_login/client.py", line 159, in login
    **kwargs)
  File "/global/homes/f/forsyth/miniconda3/envs/zstash_issue196/lib/python3.7/site-packages/fair_research_login/client.py", line 198, in get_code
    additional_params=additional_params
TypeError: oauth2_get_authorize_url() got an unexpected keyword argument 'additional_params'

----------------------------------------------------------------------
Ran 62 tests in 526.255s

FAILED (errors=1)

@forsyth2
Copy link
Collaborator Author

forsyth2 commented Apr 1, 2022

The second commit (based on lukaszlacinski@13c2660 from @lukaszlacinski) gets the tests passing again.

@forsyth2 forsyth2 marked this pull request as ready for review April 1, 2022 23:18
@forsyth2 forsyth2 merged commit 3c75ea5 into E3SM-Project:master Apr 1, 2022
@forsyth2 forsyth2 deleted the fix-sdk branch April 1, 2022 23:18
@forsyth2
Copy link
Collaborator Author

forsyth2 commented Apr 1, 2022

@xylar This closes #196, meaning zstash should now support python 3.10.

@xylar
Copy link
Contributor

xylar commented Apr 2, 2022

That's great!

@xylar
Copy link
Contributor

xylar commented Jan 27, 2023

@forsyth2, I have questions about the changes made here. It should always be that conda/meta.yaml and setup.py have the same exact constraints on packages. Otherwise, pip check will not pass. Do you know why stricter (and different) constraints were added to setup.py than in conda/meta.yaml in this PR?

@forsyth2
Copy link
Collaborator Author

forsyth2 commented Jan 27, 2023

It should always be that conda/meta.yaml and setup.py have the same exact constraints on packages.

I was unaware of that.

Do you know why stricter (and different) constraints were added to setup.py than in conda/meta.yaml in this PR?

No, I can only imagine I was going with the least strict requirements needed for it to work. @tomvothecoder or @lukaszlacinski might remember why, but I'm not sure.

@tomvothecoder
Copy link
Collaborator

tomvothecoder commented Jan 30, 2023

No, I can only imagine I was going with the least strict requirements needed for it to work. @tomvothecoder or @lukaszlacinski might remember why, but I'm not sure.

If I recall correctly,globus-sdk v2 and v3 had breaking API changes where modules were renamed and/or moved around. These breaking changes cascaded to thee Python imports in zstash. The workaround was to constrain globus-sdk >=2, <3 in setup.py in #188.

I'm not sure of the history of the dependencies since then and why things are constrained differently in setup.py and meta.yaml. It looks like the zstash conda package using meta.yaml builds fine? If so, maybe aligning the dependencies between both files will work (although we should eventually replace the pip build workflow with mamba in GitHub Actions).

@xylar
Copy link
Contributor

xylar commented Jan 30, 2023

Let's settle on the right set of constraints in #250.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DevOps CI/CD, configuration, etc. Globus Globus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support newer globus-sdk
3 participants